Skip to content

Conversation

jainanmol84
Copy link

@jainanmol84 jainanmol84 commented Mar 25, 2025

  • Commit Message Requirements
  • Built against Vault/LTS Environment
  • kABI Check Passed, where Valid (Pre 9.4 RT does not have kABI stability)
  • Boot Test
  • Kernel SelfTest results
  • Additional Tests as determined relevant

Commit message

jira VULN-46444
cve CVE-2024-53122
commit-author Paolo Abeni <[email protected]>
commit ce7356ae35943cc6494cc692e62d51a734062b7d

Additional active subflows - i.e. created by the in kernel path manager - are included into the subflow list before starting the 3whs.

A racing recvmsg() spooling data received on an already established subflow would unconditionally call tcp_cleanup_rbuf() on all the current subflows, potentially hitting a divide by zero error on the newly created ones.

Explicitly check that the subflow is in a suitable state before invoking tcp_cleanup_rbuf().

Fixes: c76c6956566f ("mptcp: call tcp_cleanup_rbuf on subflows")
	Signed-off-by: Paolo Abeni <[email protected]>
	Reviewed-by: Matthieu Baerts (NGI0) <[email protected]>
Link: https://patch.msgid.link/02374660836e1b52afc91966b7535c8c5f7bafb0.1731060874.git.pabeni@redhat.com
	Signed-off-by: Jakub Kicinski <[email protected]>
(cherry picked from commit ce7356ae35943cc6494cc692e62d51a734062b7d)
	Signed-off-by: Anmol Jain <[email protected]>

Kernel build logs

kernel-build.log

/home/anmol/kernel-src-tree
no .config file found, moving on
[TIMER]{MRPROPER}: 0s
x86_64 architecture detected, copying config
'configs/kernel-x86_64-rhel.config' -> '.config'
Setting Local Version for build
CONFIG_LOCALVERSION="-_ajain__ciqlts9_4-201ea9b33"
Making olddefconfig
  HOSTCC  scripts/basic/fixdep
  HOSTCC  scripts/kconfig/conf.o
  HOSTCC  scripts/kconfig/confdata.o
  HOSTCC  scripts/kconfig/expr.o
  LEX     scripts/kconfig/lexer.lex.c
  YACC    scripts/kconfig/parser.tab.[ch]
  HOSTCC  scripts/kconfig/lexer.lex.o
  HOSTCC  scripts/kconfig/menu.o
  HOSTCC  scripts/kconfig/parser.tab.o
  HOSTCC  scripts/kconfig/preprocess.o
  HOSTCC  scripts/kconfig/symbol.o
  HOSTCC  scripts/kconfig/util.o
  HOSTLD  scripts/kconfig/conf
#
# configuration written to .config
#
Starting Build
  SYSHDR  arch/x86/include/generated/uapi/asm/unistd_32.h
  SYSHDR  arch/x86/include/generated/uapi/asm/unistd_64.h
  SYSHDR  arch/x86/include/generated/uapi/asm/unistd_x32.h
  SYSTBL  arch/x86/include/generated/asm/syscalls_32.h
  SYSHDR  arch/x86/include/generated/asm/unistd_32_ia32.h
  SYSHDR  arch/x86/include/generated/asm/unistd_64_x32.h
  WRAP    arch/x86/include/generated/uapi/asm/bpf_perf_event.h
  WRAP    arch/x86/include/generated/uapi/asm/errno.h
  WRAP    arch/x86/include/generated/uapi/asm/fcntl.h
  WRAP    arch/x86/include/generated/uapi/asm/ioctl.h
  [---snip---]
INSTALL /lib/modules/5.14.0-_ajain__ciqlts9_4-201ea9b33+/kernel/sound/xen/snd_xen_front.ko
  STRIP   /lib/modules/5.14.0-_ajain__ciqlts9_4-201ea9b33+/kernel/sound/xen/snd_xen_front.ko
  SIGN    /lib/modules/5.14.0-_ajain__ciqlts9_4-201ea9b33+/kernel/sound/xen/snd_xen_front.ko
  INSTALL /lib/modules/5.14.0-_ajain__ciqlts9_4-201ea9b33+/kernel/virt/lib/irqbypass.ko
  STRIP   /lib/modules/5.14.0-_ajain__ciqlts9_4-201ea9b33+/kernel/virt/lib/irqbypass.ko
  SIGN    /lib/modules/5.14.0-_ajain__ciqlts9_4-201ea9b33+/kernel/virt/lib/irqbypass.ko
  DEPMOD  /lib/modules/5.14.0-_ajain__ciqlts9_4-201ea9b33+
[TIMER]{MODULES}: 42s
Making Install
sh ./arch/x86/boot/install.sh 5.14.0-_ajain__ciqlts9_4-201ea9b33+ \
        arch/x86/boot/bzImage System.map "/boot"
[TIMER]{INSTALL}: 31s
Checking kABI
kABI check passed
Setting Default Kernel to /boot/vmlinuz-5.14.0-_ajain__ciqlts9_4-201ea9b33+ and Index to 2
The default is /boot/loader/entries/c09ae7f16c7843a2947e0b4b0c2c6380-5.14.0-_ajain__ciqlts9_4-201ea9b33+.conf with index 2 and kernel /boot/vmlinuz-5.14.0-_ajain__ciqlts9_4-201ea9b33+
The default is /boot/loader/entries/c09ae7f16c7843a2947e0b4b0c2c6380-5.14.0-_ajain__ciqlts9_4-201ea9b33+.conf with index 2 and kernel /boot/vmlinuz-5.14.0-_ajain__ciqlts9_4-201ea9b33+
Generating grub configuration file ...
Adding boot menu entry for UEFI Firmware Settings ...
done
Hopefully Grub2.0 took everything ... rebooting after time metrices
[TIMER]{MRPROPER}: 0s
[TIMER]{BUILD}: 2106s
[TIMER]{MODULES}: 42s
[TIMER]{INSTALL}: 31s
[TIMER]{TOTAL} 2185s
Rebooting in 10 seconds

kernel-build.log

Kselftests

$ grep '^ok ' kselftest-before.log | wc -l && grep '^ok ' kselftest-after.log | wc -l
361
361
grep '^not ok ' kselftest-before.log | wc -l && grep '^not ok ' kselftest-after.log | wc -l
76
76

kselftest-after.log
kselftest-before.log

jira VULN-46444
cve CVE-2024-53122
commit-author Paolo Abeni <[email protected]>
commit ce7356a

Additional active subflows - i.e. created by the in kernel path
manager - are included into the subflow list before starting the
3whs.

A racing recvmsg() spooling data received on an already established
subflow would unconditionally call tcp_cleanup_rbuf() on all the
current subflows, potentially hitting a divide by zero error on
the newly created ones.

Explicitly check that the subflow is in a suitable state before
invoking tcp_cleanup_rbuf().

Fixes: c76c695 ("mptcp: call tcp_cleanup_rbuf on subflows")
	Signed-off-by: Paolo Abeni <[email protected]>
	Reviewed-by: Matthieu Baerts (NGI0) <[email protected]>
Link: https://patch.msgid.link/02374660836e1b52afc91966b7535c8c5f7bafb0.1731060874.git.pabeni@redhat.com
	Signed-off-by: Jakub Kicinski <[email protected]>
(cherry picked from commit ce7356a)
	Signed-off-by: Anmol Jain <[email protected]>
@thefossguy-ciq
Copy link

Great first PR! I noticed a few things, please rectify them.

  1. The kernel-build.log file is not attached.
  2. The kselftest-after.log file is not uploaded. Doesn't link to anything.
  3. There are two kselftest-before.log file links. The first link is to a PR and not to a file.
  4. The build log shows that the kABI check passed but you forgot to tick the check box for the kABI check (this is at the very beginning of the PR text).

You can edit the PR and,

  1. Upload/attach the kernel-build.log file.
  2. Remove the last few lines for kselftest-*.log files and re-upload them.
  3. Add an x between the [ ] brackets in the kABI Check Passed, where Valid (Pre 9.4 RT does not have kABI stability) list item.

@jainanmol84
Copy link
Author

Great first PR! I noticed a few things, please rectify them.

  1. The kernel-build.log file is not attached.
  2. The kselftest-after.log file is not uploaded. Doesn't link to anything.
  3. There are two kselftest-before.log file links. The first link is to a PR and not to a file.
  4. The build log shows that the kABI check passed but you forgot to tick the check box for the kABI check (this is at the very beginning of the PR text).

You can edit the PR and,

  1. Upload/attach the kernel-build.log file.
  2. Remove the last few lines for kselftest-*.log files and re-upload them.
  3. Add an x between the [ ] brackets in the kABI Check Passed, where Valid (Pre 9.4 RT does not have kABI stability) list item.
  1. kernel-build.log file is already there - mptcp: cope racing subflow creation in mptcp_rcv_space_adjust #175

  2. I've updated kselftest-after.log` file. There was a syntax mistake.

Copy link

@thefossguy-ciq thefossguy-ciq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚤

@bmastbergen bmastbergen self-requested a review March 26, 2025 14:00
Copy link
Collaborator

@bmastbergen bmastbergen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! @jainanmol84 🥌

@PlaidCat
Copy link
Collaborator

I looked at this and got a merge conflict on the fuzz above the conflicted change.

 tcp_sk(ssk)->window_clamp = window_clamp;
vs
 WRITE_ONCE(tcp_sk(ssk)->window_clamp, window_clamp);

Because the ssk data is used in both lines I investigated why they added the annotations:
torvalds/linux@f410cbe

Since it looks like they're exclusively referring to window_clamp and data race I didn't investigate to see if we should bring in this commit for annotation as it would require us to validate that all the assurances that that ssk->window_clam assertions are true that it can be read locklessly, as the sk_rcvbuf has these annotations without window_clamp.

I'm not going to block this for an update since we don't have a codified method of dealing with conflicts in surrounding code but usually if the code in conflict shares the same variables / symbols we might want to mention it, even if its not in the commit message and what we learned.

Maybe we add this to the PR as like an due diligence check, I could open a discussion on an Issue for public committers to
weigh in?

@bmastbergen
Copy link
Collaborator

I'm not going to block this for an update since we don't have a codified method of dealing with conflicts in surrounding code but usually if the code in conflict shares the same variables / symbols we might want to mention it, even if its not in the commit message and what we learned.

Maybe we add this to the PR as like an due diligence check, I could open a discussion on an Issue for public committers to weigh in?

Mentioning merge conflicts for surrounding code in the PR seems like a good idea. I've typically not mentioned them in the 'upstream-diff' section of the commit message with the justification that they technically aren't a difference from the code that was changed in the upstream commit. However I do look at those surrounding conflicts and make a determination about whether they are relevant or not. Not mentioning them at all might prevent the case where a reviewer makes a different determination as to their relevance than I did.

In this particular case I also noticed the surrounding difference and also determined that I didn't think they were relevant to the changeset under review.

TLDR; For backported code, let's mention merge conflicts in the code surrounding the code change under review at least in the PR, or even in an 'upstream-diff' commit log section if you think it might be useful to a future commit log reader.

Merge away @jainanmol84

@solardiz
Copy link

I looked at this and got a merge conflict on the fuzz above the conflicted change.

 tcp_sk(ssk)->window_clamp = window_clamp;
vs
 WRITE_ONCE(tcp_sk(ssk)->window_clamp, window_clamp);

FWIW, the upstream stable backport to 6.1.119 mentioned in https://lore.kernel.org/linux-cve-announce/2024120252-CVE-2024-53122-f35c@gregkh/T/#u also has this without the WRITE_ONCE, so at least we won't be alone with this combination (of no WRITE_ONCE addition and this fix):

$ git show 24995851d58c --pretty=fuller
commit 24995851d58c4a205ad0ffa7b2f21e479a9c8527
Author:     Paolo Abeni <[email protected]>
AuthorDate: Tue Nov 19 09:35:49 2024 +0100
Commit:     Greg Kroah-Hartman <[email protected]>
CommitDate: Fri Nov 22 15:37:33 2024 +0100

    mptcp: cope racing subflow creation in mptcp_rcv_space_adjust
    
    commit ce7356ae35943cc6494cc692e62d51a734062b7d upstream.
    
    Additional active subflows - i.e. created by the in kernel path
    manager - are included into the subflow list before starting the
    3whs.
    
    A racing recvmsg() spooling data received on an already established
    subflow would unconditionally call tcp_cleanup_rbuf() on all the
    current subflows, potentially hitting a divide by zero error on
    the newly created ones.
    
    Explicitly check that the subflow is in a suitable state before
    invoking tcp_cleanup_rbuf().
    
    Fixes: c76c6956566f ("mptcp: call tcp_cleanup_rbuf on subflows")
    Signed-off-by: Paolo Abeni <[email protected]>
    Reviewed-by: Matthieu Baerts (NGI0) <[email protected]>
    Link: https://patch.msgid.link/02374660836e1b52afc91966b7535c8c5f7bafb0.1731060874.git.pabeni@redhat.com
    Signed-off-by: Jakub Kicinski <[email protected]>
    [ Conflicts in protocol.c, because commit f410cbea9f3d ("tcp: annotate
      data-races around tp->window_clamp") has not been backported to this
      version. The conflict is easy to resolve, because only the context is
      different, but not the line to modify. ]
    Signed-off-by: Matthieu Baerts (NGI0) <[email protected]>
    Signed-off-by: Greg Kroah-Hartman <[email protected]>

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 78ac5c538e13..1acd4e37a0ea 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -2057,7 +2057,8 @@ static void mptcp_rcv_space_adjust(struct mptcp_sock *msk, int copied)
                                slow = lock_sock_fast(ssk);
                                WRITE_ONCE(ssk->sk_rcvbuf, rcvbuf);
                                tcp_sk(ssk)->window_clamp = window_clamp;
-                               tcp_cleanup_rbuf(ssk, 1);
+                               if (tcp_can_send_ack(ssk))
+                                       tcp_cleanup_rbuf(ssk, 1);
                                unlock_sock_fast(ssk, slow);
                        }
                }

What's important is to not try backporting this fix to kernels that don't have the commit listed in Fixes. According to RH, they fixed this even for EL8, so perhaps they had backported the buggy commit to there - but this doesn't tell us when they did that. We may have 8.x LTS branches that don't have that, so need to be careful to check first.

@solardiz
Copy link

TLDR; For backported code, let's mention merge conflicts in the code surrounding the code change under review at least in the PR, or even in an 'upstream-diff' commit log section if you think it might be useful to a future commit log reader.

Here's how the same conflict is mentioned in the upstream stable commit I quoted above:

    [ Conflicts in protocol.c, because commit f410cbea9f3d ("tcp: annotate
      data-races around tp->window_clamp") has not been backported to this
      version. The conflict is easy to resolve, because only the context is
      different, but not the line to modify. ]

@thefossguy-ciq
Copy link

Re: Merge conflict

This PR exists as the result of me helping Amol get familiar with the Kernel CVE process by making her fix one CVE and assisting in that. I focused on the process of it and not much on the CVE fix itself because I believe that Anmol can/should handle that, given her experience with userspace CVEs.

Given that the issue was introduced with v5.10, when we were presented with a merge conflict, I suggested that we pull in the patch from v5.15--the closest longterm tree. The commit's message is generated by our cherry pick script but the actual diff is from the patch merged in the v5.15 tree: https://web.git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/net/mptcp/protocol.c?h=v5.15.179&id=0a9a182ea5c7bb0374e527130fd85024ace7279b

+1 on mentioning if there is a merge conflict and the engineer's solution for it.

@PlaidCat
Copy link
Collaborator

As long as we always use the Upstream Commit SHA but you can see that RedHat made changes from the 5.10. so a different line would have been in conflict but ultimately still in contextual information.

I have updates I need to do to the WIKI anyways to remove RT stuff so maybe I'll include that if everyone generally agrees.

@jainanmol84 jainanmol84 merged commit c58cea1 into ciqlts9_4 Mar 27, 2025
4 checks passed
@jainanmol84 jainanmol84 deleted the {ajain}_ciqlts9_4 branch March 27, 2025 15:54
github-actions bot pushed a commit that referenced this pull request May 3, 2025
Commit <5518f239aff1> ("iommu/vt-d: Move scalable mode ATS enablement to
probe path") changed the PCI ATS enablement logic to run earlier,
specifically before the default domain attachment.

On some client platforms, this change resulted in boot failures, causing
the kernel to panic with the following message and call trace:

 Kernel panic - not syncing: DMAR hardware is malfunctioning
 CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.14.0-rc3+ #175
 Call Trace:
  <TASK>
  dump_stack_lvl+0x6f/0xb0
  dump_stack+0x10/0x16
  panic+0x10a/0x2b7
  iommu_enable_translation.cold+0xc/0xc
  intel_iommu_init+0xe39/0xec0
  ? trace_hardirqs_on+0x1e/0xd0
  ? __pfx_pci_iommu_init+0x10/0x10
  pci_iommu_init+0xd/0x40
  do_one_initcall+0x5b/0x390
  kernel_init_freeable+0x26d/0x2b0
  ? __pfx_kernel_init+0x10/0x10
  kernel_init+0x15/0x120
  ret_from_fork+0x35/0x60
  ? __pfx_kernel_init+0x10/0x10
  ret_from_fork_asm+0x1a/0x30
 RIP: 1f0f:0x0
 Code: Unable to access opcode bytes at 0xffffffffffffffd6.
 RSP: 0000:0000000000000000 EFLAGS: 841f0f2e66 ORIG_RAX:
      1f0f2e6600000000
 RAX: 0000000000000000 RBX: 1f0f2e6600000000 RCX:
      2e66000000000084
 RDX: 0000000000841f0f RSI: 000000841f0f2e66 RDI:
      00841f0f2e660000
 RBP: 00841f0f2e660000 R08: 00841f0f2e660000 R09:
      000000841f0f2e66
 R10: 0000000000841f0f R11: 2e66000000000084 R12:
      000000841f0f2e66
 R13: 0000000000841f0f R14: 2e66000000000084 R15:
      1f0f2e6600000000
  </TASK>
 ---[ end Kernel panic - not syncing: DMAR hardware is malfunctioning ]---

Fix this by reverting the timing change for ATS enablement introduced by
the offending commit and restoring the previous behavior.

Fixes: 5518f23 ("iommu/vt-d: Move scalable mode ATS enablement to probe path")
Reported-by: Jarkko Nikula <[email protected]>
Closes: https://lore.kernel.org/linux-iommu/[email protected]/
Signed-off-by: Lu Baolu <[email protected]>
Tested-by: Jarkko Nikula <[email protected]>
Reviewed-by: Kevin Tian <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Joerg Roedel <[email protected]>
github-actions bot pushed a commit that referenced this pull request Jul 25, 2025
commit 4f1492e
Author: Lu Baolu <[email protected]>
Date: Wed Apr 16 15:36:08 2025 +0800

    iommu/vt-d: Revert ATS timing change to fix boot failure

    Commit <5518f239aff1> ("iommu/vt-d: Move scalable mode ATS enablement to
    probe path") changed the PCI ATS enablement logic to run earlier,
    specifically before the default domain attachment.

    On some client platforms, this change resulted in boot failures, causing
    the kernel to panic with the following message and call trace:

     Kernel panic - not syncing: DMAR hardware is malfunctioning
     CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.14.0-rc3+ #175
     Call Trace:
      <TASK>
      dump_stack_lvl+0x6f/0xb0
      dump_stack+0x10/0x16
      panic+0x10a/0x2b7
      iommu_enable_translation.cold+0xc/0xc
      intel_iommu_init+0xe39/0xec0
      ? trace_hardirqs_on+0x1e/0xd0
      ? __pfx_pci_iommu_init+0x10/0x10
      pci_iommu_init+0xd/0x40
      do_one_initcall+0x5b/0x390
      kernel_init_freeable+0x26d/0x2b0
      ? __pfx_kernel_init+0x10/0x10
      kernel_init+0x15/0x120
      ret_from_fork+0x35/0x60
      ? __pfx_kernel_init+0x10/0x10
      ret_from_fork_asm+0x1a/0x30
     RIP: 1f0f:0x0
     Code: Unable to access opcode bytes at 0xffffffffffffffd6.
     RSP: 0000:0000000000000000 EFLAGS: 841f0f2e66 ORIG_RAX:
	  1f0f2e6600000000
     RAX: 0000000000000000 RBX: 1f0f2e6600000000 RCX:
	  2e66000000000084
     RDX: 0000000000841f0f RSI: 000000841f0f2e66 RDI:
	  00841f0f2e660000
     RBP: 00841f0f2e660000 R08: 00841f0f2e660000 R09:
	  000000841f0f2e66
     R10: 0000000000841f0f R11: 2e66000000000084 R12:
	  000000841f0f2e66
     R13: 0000000000841f0f R14: 2e66000000000084 R15:
	  1f0f2e6600000000
      </TASK>
     ---[ end Kernel panic - not syncing: DMAR hardware is malfunctioning ]---

    Fix this by reverting the timing change for ATS enablement introduced by
    the offending commit and restoring the previous behavior.

    Fixes: 5518f23 ("iommu/vt-d: Move scalable mode ATS enablement to probe path")
    Reported-by: Jarkko Nikula <[email protected]>
    Closes: https://lore.kernel.org/linux-iommu/[email protected]/
    Signed-off-by: Lu Baolu <[email protected]>
    Tested-by: Jarkko Nikula <[email protected]>
    Reviewed-by: Kevin Tian <[email protected]>
    Link: https://lore.kernel.org/r/[email protected]
    Signed-off-by: Joerg Roedel <[email protected]>

(cherry picked from commit 4f1492e)
Upstream-Status: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
Signed-off-by: Jerry Snitselaar <[email protected]>
JIRA: https://issues.redhat.com/browse/RHEL-89891
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

5 participants